Skip to content

feat: Scope KMS policy to the exact KMS key #295

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 22 commits into from
Mar 25, 2025
Merged

feat: Scope KMS policy to the exact KMS key #295

merged 22 commits into from
Mar 25, 2025

Conversation

arya-girish-k
Copy link
Contributor

@arya-girish-k arya-girish-k commented Feb 27, 2025 •
edited by Aayush-Abhyarthi
Loading

Description

Scope KMS policy to the exact KMS key
Git_issue.

Release required?

  • No release
  • Patch release (x.x.X)
  • Minor release (x.X.x)
  • Major release (X.x.x)
Release notes content

As part of addressing this issue,made the following modification:

  1. Updated the KMS policy and added an additional policy for the HPCS key in the root module and DA.
  2. Introduced a boolean variable is_hpcs_key to create the second policy.
  3. Implemented a CRN parser to retrieve the kms_instance_guid from kms_key_crn_parser, removing the existing_kms_instance_guid.
  4. Updated the validation

Run the pipeline

If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.

Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:

/run pipeline

Checklist for reviewers

  • If relevant, a test for the change is included or updated with this PR.
  • If relevant, documentation for the change is included or updated with this PR.

For mergers

  • Use a conventional commit message to set the release level. Follow the guidelines.
  • Include information that users need to know about the PR in the commit message. The commit message becomes part of the GitHub release notes.
  • Use the Squash and merge option.

@arya-girish-k arya-girish-k marked this pull request as draft February 27, 2025 06:37
@arya-girish-k arya-girish-k changed the title feat: Scope KMS policy to the exact KMS key (Do not merge):feat: Scope KMS policy to the exact KMS key Feb 27, 2025
@ocofaigh
Copy link
Contributor

@arya-girish-k any update on this?

ocofaigh
ocofaigh reviewed Mar 18, 2025
View reviewed changes
ocofaigh
ocofaigh reviewed Mar 18, 2025
View reviewed changes
ocofaigh
ocofaigh requested changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets forget about DA updates in this PR - the DA is being refactored in #300 and it will handle the DA updates

@ocofaigh ocofaigh mentioned this pull request Mar 18, 2025
Merged
6 tasks
ocofaigh
ocofaigh requested changes Mar 18, 2025
View reviewed changes
Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@arya-girish-k arya-girish-k changed the title (Do not merge):feat: Scope KMS policy to the exact KMS key feat: Scope KMS policy to the exact KMS key Mar 21, 2025
@arya-girish-k
Copy link
Contributor Author

1.Added two policies in the rootmodule and DA.
2.Added crn parser ,since retriving kms_instance_guid from kms_key_crn_parser,removed existing_kms_instance_guid.
3.Added boolean variable is_hpcs_key to create the second policy
4.Faced the below error while testing:

│   CreateNotificationsRegistrationWithContext failed Invalid authorization with Event Notifications instance

Since it is Known error, removed this line in complete example.

@arya-girish-k
Copy link
Contributor Author

/run pipeline

@arya-girish-k
Copy link
Contributor Author

/run pipeline

@arya-girish-k
Copy link
Contributor Author

As expected, the upgrade test fails due to the re-creation of the auth policy, however since we are using create_before_destroy = true there will be no disruption to key access so skipping upgrade test.
image

@arya-girish-k
Copy link
Contributor Author

/run pipeline

@arya-girish-k arya-girish-k requested a review from ocofaigh March 24, 2025 07:56
ocofaigh
ocofaigh requested changes Mar 24, 2025
View reviewed changes
Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see final comments

@Aayush-Abhyarthi
Copy link
Member

/run pipeline

@Aayush-Abhyarthi Aayush-Abhyarthi marked this pull request as ready for review March 25, 2025 12:07
ocofaigh
ocofaigh requested changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just realised we don't need the new boolean in the DA - we can parse the CRN (see comment)

@Aayush-Abhyarthi
Copy link
Member

/run pipeline

@Aayush-Abhyarthi
Copy link
Member

/run pipeline

ocofaigh
ocofaigh requested changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@ocofaigh ocofaigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to be missing logic in the module that verifies if is_hpcs_key is set to true, it matches the instance type in the kms_key_crn value. Can you add it please?

@Aayush-Abhyarthi
Copy link
Member

/run pipeline

@ocofaigh ocofaigh merged commit a0cab06 into main Mar 25, 2025
2 checks passed
@ocofaigh ocofaigh deleted the 11349-scope branch March 25, 2025 20:01
@terraform-ibm-modules-ops
Copy link
Contributor

🎉 This PR is included in version 1.26.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocofaigh ocofaigh ocofaigh approved these changes

@alex-reiff alex-reiff Awaiting requested review from alex-reiff alex-reiff is a code owner

@shemau shemau Awaiting requested review from shemau shemau is a code owner

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arya-girish-k @ocofaigh @Aayush-Abhyarthi @terraform-ibm-modules-ops